Codecov Report

Merging #704 (2a6c423) into main (d36f676) will increase coverage by 0.4%.
The diff coverage is 98.4%.

@@           Coverage Diff           @@
##            main    #704     +/-   ##
=======================================
+ Coverage   94.8%   95.2%   +0.4%     
=======================================
  Files         43      46      +3     
  Lines       3797    4312    +515     
=======================================
+ Hits        3600    4107    +507     
- Misses       197     205      +8     

Location: I propose to drop the MCA level from the tree, i.e., to move LPdiag dir directly below the tools. I would keep the LPdiag name unless someone will propose a better name.

Alternative names for possible consideration: LPcheck, LPinfo.

This was solved. The tool is called lp_diag, and it is moved to the right place: .../ tools/lp_diag

Unfortunately, I cannot contribute to the PR review:

• The code works on iMac (ventura) and MacBook in pyCharm with py 3.10. I have no idea what/why errors occur on other OSs
• I will extend the functionality after having suggestions from colleagues on what is desired.

I will update the readme.txt files, and later write some documentation. However, I assume that pushing any changes should wait until the initial PR will be accepted and the files will be in a better place than pr/704

Unfortunately, I couldn't (without significantly more work) keep the commit history and move the files to their new location. Please let me know if that should still be done.

Thanks for moving the stuff. Retrieving the commit history is certainly not worth additional work.

All committers have signed the CLA.

Thanks for the updates :)
A few things to note:

• You added some .gitignore files. In general, these shared exclusion files should only contain exclusion patterns that other people might also reasonably have. For example, the .swp you added to the main .gitignore seems like the exclusion of a certain file type that others should not commit, either. But of_*, as you specified in message_ix/tools/lp_diag/data/mps/.gitignore, seems like these are only files that concern yourself locally (and same for all directories listed in message_ix/tools/lp_diag/.gitignore). While we don't have a strict rule for this, I think it is strongly preferred to exclude these files only locally. You can achieve this by adding them to the file .git/info/exclude (see the docs here).
• message_ix/tools/lp_diag/Readme.txt includes some mention of a directory that will not be accessible to the public, so there is not much gained by mentioning it. Could you please rework that section? For sharing large files, the git large file storage might be an option (but that could also clutter our repositories) or you might be interested in checking out zenodo.
• The main format we use for documentation is restructured text (rst). This grants additional features like links and more formatting options than txt. For the README files that are only intended to show information on GitHub, txt might be fine, but our landing page is an md file, which also has more options than txt. For the documentation itself, rst is necessary since the docs are built automatically using sphinx.
• Also regarding the documentation, is there any specific reason not to include a short description of how the tool currently works? If we merge the PR now, we will release this tool without people knowing how to use it or what to use it for; it will take another PR to update the documentation and things like scope creep are real: you start a PR for working on the docs, but you keep finding little things to include and so the PR doesn't get merged for a long time. Please include some documentation, however preliminary, here.
• Please note that there is no need to repeat yourself: compare the add_year tool. The doc/tools/add_year.rst is simply a reference to the rst file that had already been written. You could do the same here.
• About adding to the release notes, I can add a note that this tool is now included with message_ix. Please just let me know how to describe it properly (ideally in one line), something like: Introduce the LPdiag tool to ...

Thanks for the updates :) A few things to note:

Thanks for comments and suggestions. Please see my reaction to each issue below:

• You added some .gitignore files. In general, these shared exclusion files should only contain exclusion patterns that other people might also reasonably have. For example, the .swp you added to the main .gitignore seems like the exclusion of a certain file type that others should not commit, either. But of_*, as you specified in message_ix/tools/lp_diag/data/mps/.gitignore, seems like these are only files that concern yourself locally (and same for all directories listed in message_ix/tools/lp_diag/.gitignore). While we don't have a strict rule for this, I think it is strongly preferred to exclude these files only locally. You can achieve this by adding them to the file .git/info/exclude (see the docs here).

I will review and implement exclusions of the files that indeed should be only local. BTW: I am indeed convinced that nobody would like to commit the *.swp files.

• message_ix/tools/lp_diag/Readme.txt includes some mention of a directory that will not be accessible to the public, so there is not much gained by mentioning it. Could you please rework that section? For sharing large files, the git large file storage might be an option (but that could also clutter our repositories) or you might be interested in checking out zenodo.

I will review and correct this.

• The main format we use for documentation is restructured text (rst). This grants additional features like links and more formatting options than txt. For the README files that are only intended to show information on GitHub, txt might be fine, but our landing page is an md file, which also has more options than txt. For the documentation itself, rst is necessary since the docs are built automatically using sphinx.

I have already explored sphinx a little bit, and will write the *rst version after we will clarify several small issues pointed out below.

• Also regarding the documentation, is there any specific reason not to include a short description of how the tool currently works? If we merge the PR now, we will release this tool without people knowing how to use it or what to use it for; it will take another PR to update the documentation and things like scope creep are real: you start a PR for working on the docs, but you keep finding little things to include and so the PR doesn't get merged for a long time. Please include some documentation, however preliminary, here.

Indeed, we should wait with merging until the documentation will be in a good enough shape. Regarding the usage: I want to discuss this issue with at least Oliver as I have only the developer perspective. We also need to add the a hint on generating the MPS-format file from a message_ix generated model instance; also here I need help of Oliver.

• Please note that there is no need to repeat yourself: compare the add_year tool. The doc/tools/add_year.rst is simply a reference to the rst file that had already been written. You could do the same here.

I will keep this in mind when writing *rst. BTW: please let me know, how to locally generate (using the message_ix style/config) the resulting *html version?

• About adding to the release notes, I can add a note that this tool is now included with message_ix. Please just let me know how to describe it properly (ideally in one line), something like: Introduce the LPdiag tool to ...

I would consider:
Introduce the LPdiag tool to support analysis of numerical characteristics of the generated optimization problem.

Also here Oliver, and possibly other colleagues, may have better ideas.

Thanks, Marek, for your detailed answer. Just pinging @OFR-IIASA here to put this PR on this radar.

For building the sphinx docs locally, we have this guide. Please let me know if this is not complete or needs updates.

Dear @marek-iiasa and @measrainsey, please take a look at this PR again. While adding some tests today, I restructured some aspects of the tool, so please make sure it still behaves as intended. Among others, I renamed some functions (e.g., lpdiag.stat() to lpdiag.print_statistics()) so that the names are more intuitive to use. I've also converted the general raise Exceptions and some prints to raising specific error messages, so the code might fail on more occasions than before.
Apart from that, the code here is up-to-date with main and has enough test coverage now to be merged. So we can do that if you have no more additions to the docs or similar. Of course, there is probably also a more elegant way to test these error cases, feel free to update the existing tests as you like.
Please let me know what you think :)

Please note: you can click on 'Show all checks', find the one called 'docs/readthedocs.com:iiasa-energy-program-message-ix' and click on 'Details' to see the docs as built from the files in this PR :)
However, maybe the current build was triggered too soon after the last successful one, the changes from 'Update documentation' don't seem to be included yet.

I've updated the PR to comply with your suggestions, thanks @khaeru. For the last point, though, I think we will need your input, @marek-iiasa.

One note about .gitignore files: there are two of them in tutorial and tutorial/westeros, so two more that are not on the top level. Should these also be merged with the top level one?

Thanks for your contribution here, @ywpratama. Unfortunately, your commit broke several tests ... as does mine. I thought this might be fixed by rebasing on top of main, but apparently, that's not the case. I will have to investigate. Sorry for overwriting your commit.